Skip to content

Conversation

@micol-altomare
Copy link

@micol-altomare micol-altomare commented Nov 26, 2025

Replace ensure_clean_store with tmp_path fixture in the following files:

  • pandas/tests/io/pytables/test_keys.py
  • pandas/tests/io/pytables/test_complex.py
  • pandas/tests/io/pytables/test_file_handling.py

Specifically,

  • Replace all occurrences of ensure_clean_store with + HDFStore(tmp_path / setup_path)
  • Removed ensure_clean_store imports (no longer used)
  • Added tmp_path parameter to modified test functions

=====

  • Code fixes #xxx
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.
  • If I used AI to develop this pull request, I prompted it to follow AGENTS.md.

@micol-altomare micol-altomare force-pushed the micol-altomare-replace-ensure-clean-store branch from f981616 to 1b5b02c Compare November 27, 2025 17:00
@micol-altomare micol-altomare marked this pull request as draft November 29, 2025 18:25
@micol-altomare micol-altomare marked this pull request as ready for review November 29, 2025 18:25
@micol-altomare micol-altomare changed the title TST: Replace ensure_clean_store with tmp_path in test_keys.py, test_complex.py, test_file_handling.py TST: Replace ensure_clean_store with tmp_path in test_complex.py, test_file_handling.py Nov 29, 2025
@micol-altomare
Copy link
Author

Hi @mroeschke! 👋 Would you mind reviewing this PR (for #62435) and letting me know if there are any changes needed on my end in order to successfully merge? Thank you in advance!

@micol-altomare
Copy link
Author

Hi @rhshadrach and @mroeschke, just wanted to kindly request feedback from either of you whenever you have a chance. I appreciate you both taking a look; whoever is able to review is perfect, and I’m happy to respond to any feedback. Thanks so much!

from pandas.tests.io.pytables.common import ensure_clean_store

from pandas.io.pytables import read_hdf
tables = pytest.importorskip("tables")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added?

Copy link
Author

@micol-altomare micol-altomare Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mroeschke I added that line because pytables (and its HDF5 dependency) aren't available in the Pyodide environment. Thus it skips those tests but keeps full test coverage where the dependency is present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is effectively called in pandas/tests/io/pytables/common.py already so it's not needed here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mroeschke Thanks! I have removed the importorskip("tables") and included from pandas.tests.io.pytables.common import tables.

from pandas.tests.io.pytables.common import tables

_ = tables

However, I'm seeing this error frequently come up: Error: ENOENT: no such file or directory, lstat '/home/runner/work/_temp/setup-micromamba/micromamba-shell' This appears to be an issue with the post-run cleanup phase and seems unrelated to my changes. Could you please trigger a retry of the CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove these lines all together. I would consider this an unrelated change that can be addressed in a follow up if needed

Copy link
Author

@micol-altomare micol-altomare Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mroeschke Awesome, I just deleted those two lines as requested.

I still have failing automated tests, but I noticed that they're the same as what's failing in the main branch so they're unrelated to my changes. I believe it's ready for another review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants